feat(core): Make Postgres connection timeout configurable #10670
Conversation
n8n
|
||||||||||||||||||||||||||||
| Project |
n8n
|
| Branch Review |
postgres-connection-timeout
|
| Run status |
|
| Run duration | 04m 44s |
| Commit |
|
| Committer | Iván Ovejero |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
1
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
424
|
| View all changes introduced in this branch ↗︎ | |
|
✅ All Cypress E2E specs passed |
n8n-assistant
bot
added
core
|
✅ All Cypress E2E specs passed |
| @@ -75,6 +75,10 @@ class PostgresConfig { | |||
| @Env('DB_POSTGRESDB_POOL_SIZE') | |||
| poolSize: number = 2; | |||
|
|
|||
| /** Postgres connection timeout (ms) */ | |||
| @Env('DB_POSTGRESDB_CONNECTION_TIMEOUT') | |||
| connectionTimeoutMs: number = 1000; | |||
There was a problem hiding this comment.
1s is a very short default value. Something like 10-30s would make more sense. Also remember to update docs
There was a problem hiding this comment.
According to Adi already 5s was too long? #10670 (comment)
The docs PR is already linked above: n8n-io/n8n-docs#2452
There was a problem hiding this comment.
typeorm docs say
The milliseconds before a timeout occurs during the initial connection to the postgres server.
So this is only the timeout for opening the connection. If that takes longer than 1 second, we might have many other issues in the setup. Setting this to 30s might not be a great idea.
There was a problem hiding this comment.
typeorm passes that into connectionTimeoutMillis for pg lib's Pool. So it includes the whole TCP connection opening, TLS handshake, PG wire protocol including authentication etc. I fear users might run into bunch of issues if the default is very tight
There was a problem hiding this comment.
it includes the whole TCP connection opening, TLS handshake, PG wire protocol including authentication
all of that should not take more than a second, even under heavy load. If it does, there are fundamental infrastructure issues that need to be addressed, instead of letting all of this take 30 seconds before timing out.
There was a problem hiding this comment.
You assume everyone is running n8n with colocated DB. And even in that case the connection can take longer if the DB is under load. I can't start n8n on my local machine if I'm using a PG database that is located in Azure West Germany:
[Node] Error: Connection terminated due to connection timeout
[Node] at Connection.<anonymous> (/n8n/node_modules/.pnpm/[email protected]/node_modules/pg/lib/client.js:132:73)
[Node] at Object.onceWrapper (node:events:633:28)
[Node] at Connection.emit (node:events:519:28)
[Node] at Connection.emit (node:domain:488:12)
[Node] at Socket.<anonymous> (/n8n/node_modules/.pnpm/[email protected]/node_modules/pg/lib/connection.js:63:12)
[Node] at Socket.emit (node:events:519:28)
[Node] at Socket.emit (node:domain:488:12)
[Node] at TCP.<anonymous> (node:net:338:12) undefined
[Node] [nodemon] app crashed - waiting for file changes before starting...
Not saying it needs to be 30s, but even something like 5s would be more reasonable
There was a problem hiding this comment.
Well, the question is do we really want to support a scenario like this with large latencies by default, or if anyone with a setup like this should have to manually set DB_POSTGRESDB_CONNECTION_TIMEOUT to a much higher value?
IMO, we should officially only support colocated databases, to avoid having to support all the other issues that large latencies might cause.
|
Got released with |
Set a configurable connection timeout for Postgres, to more gracefully handle network issues leading to reconnection attempts.